fix(bundler): resolve cnpmcore pack blockers#5911
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds runtime exports for EggAppConfig/Config and a runtime Logger re-export; updates tests to assert those runtime exports; removes ChangesRuntime exports and tests
Bundler: module-loader, framework specifier, and externals
Sequence Diagram(s)sequenceDiagram
participant Worker as Generated Worker Entry
participant Global as globalThis
participant BundleMap as __BUNDLE_MAP
participant Runtime as Module Importer
Worker->>Global: set __EGG_BUNDLE_MODULE_LOADER__ = (filepath) => __BUNDLE_MAP[normalizedKey]
Worker->>Worker: call startEgg({ baseDir, mode: 'single' })
Runtime->>Global: invoke __EGG_BUNDLE_MODULE_LOADER__(filepath)
Global->>BundleMap: normalize filepath -> lookup module
BundleMap-->>Runtime: return module
Runtime-->>Worker: module resolved from bundle
sequenceDiagram
participant Resolver as ExternalsResolver
participant RootPkg as Root package deps
participant Pkg as Installed dependency
participant NodeMods as node_modules FS
participant Result as externals map
Resolver->>RootPkg: iterate dependencies
loop per dependency
Resolver->>Pkg: read package.json (peerDependencies, peerDependenciesMeta)
Resolver->>NodeMods: find peer package dir relative to Pkg (fromDir)
alt missing optional peers
Resolver->>Result: add missing optional peer(s) to externals
else
Resolver->>Resolver: evaluate native-binary and other rules
end
end
Resolver-->>Result: return final externals map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying egg with
|
| Latest commit: |
a054390
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://19abf11c.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-a5ecaa1b.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5911 +/- ##
==========================================
- Coverage 85.04% 85.03% -0.01%
==========================================
Files 665 667 +2
Lines 19108 19110 +2
Branches 3719 3719
==========================================
+ Hits 16250 16251 +1
- Misses 2465 2466 +1
Partials 393 393 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to the egg package and the egg-bundler tool. Key changes include exposing EggAppConfig at runtime to support decorator metadata, refining the export of Logger, and removing the dependency on @eggjs/utils in the bundler by using a global loader. Additionally, the egg-bundler now correctly handles missing optional peer dependencies by externalizing them and can resolve package names from absolute framework paths. I have no feedback to provide.
Deploying egg-v3 with
|
| Latest commit: |
a054390
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7ab8df0b.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-a5ecaa1b.egg-v3.pages.dev |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes several production-bundle blockers for cnpmcore by making the generated bundle entry more portable, reducing runtime coupling, improving bundler externals detection for optional peers, and ensuring certain framework symbols exist at runtime.
Changes:
- Update
EntryGeneratorto (a) avoid absolute framework import paths by preferring the framework package name and (b) register the bundle module loader viaglobalThis.__EGG_BUNDLE_MODULE_LOADER__(dropping the@eggjs/utilsdependency). - Extend
ExternalsResolverto externalize packages that have missing optional peerDependencies and to add those missing optional peers to externals. - Add runtime exports for
EggAppConfigandLoggerfromeggso non-import typeusage survives bundler/static-export checks, with corresponding test/snapshot updates.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/EntryGenerator.ts | Makes worker entry portable (framework import) and inlines bundle-loader registration via globalThis. |
| tools/egg-bundler/package.json | Drops @eggjs/utils dependency now that worker output no longer imports it. |
| tools/egg-bundler/src/lib/ExternalsResolver.ts | Adds detection for missing optional peerDependencies and externalizes missing optional peers. |
| tools/egg-bundler/test/ExternalsResolver.test.ts | Adds coverage for missing optional peerDependency externalization and inline override behavior. |
| tools/egg-bundler/test/fixtures/externals/basic-app/package.json | Adds optional-peer-host to fixture dependencies. |
| tools/egg-bundler/test/fixtures/externals/basic-app/node_modules/optional-peer-host/package.json | New fixture package declaring optional/required peers for resolver tests. |
| tools/egg-bundler/test/EntryGenerator.test.ts | Updates assertions to match __EGG_BUNDLE_MODULE_LOADER__ registration. |
| tools/egg-bundler/test/snapshots/EntryGenerator.worker.canonical.snap | Updates canonical worker snapshot for the new loader registration approach. |
| packages/egg/src/lib/types.ts | Adds runtime EggAppConfig export (value) while keeping the interface as the type. |
| packages/egg/src/index.ts | Exposes runtime Logger export (value) from egg-logger and keeps type exports. |
| packages/egg/test/index.test.ts | Asserts runtime presence of EggAppConfig and Logger. |
| packages/egg/test/snapshots/index.test.ts.snap | Snapshot update reflecting new runtime exports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/egg-bundler/src/lib/ExternalsResolver.ts (1)
53-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
#addMissingOptionalPeerExternalsis silently skipped forforce-listed packages.When a package appears in both
depsandforce, theforceloop populatesresult[name]first. The subsequentif (result[name]) continueat line 55 then short-circuits before reaching line 59, so the optional peer propagation for that package never runs.Concrete scenario:
force: ['optional-peer-host']→missing-optional-peeris never added to externals, contrary to the automatic behavior when the same package is a plain dep.Moving the call up resolves this:
🐛 Proposed fix
for (const name of deps) { if (this.#inline.has(name) && !this.#force.has(name)) continue; + await this.#addMissingOptionalPeerExternals(name, result); if (result[name]) continue; if (await this.#shouldExternalize(name, optionalDeps, peerDeps)) { result[name] = name; } - await this.#addMissingOptionalPeerExternals(name, result); }
#addMissingOptionalPeerExternalsalready guards withif (result[peerName]) continue, so calling it earlier is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bundler/src/lib/ExternalsResolver.ts` around lines 53 - 60, The loop over deps skips calling `#addMissingOptionalPeerExternals` for packages already set in result (e.g., ones added by the force set), so optional peers of force-listed packages never get propagated; to fix, move the await this.#addMissingOptionalPeerExternals(name, result) call before the if (result[name]) continue check (or right after the inline/force skip check) so optional peer propagation runs even for force-listed packages—#addMissingOptionalPeerExternals already guards against duplicate peers, so this is safe; locate the loop using deps, this.#inline, this.#force, result and `#shouldExternalize` to apply the change.
🧹 Nitpick comments (1)
packages/egg/test/index.test.ts (1)
8-9: ⚡ Quick winUse
assertfor the new non-snapshot checks.These added assertions extend the public API contract, so it would be better to express them with the repo's standard assertion style and keep
expectonly for the snapshot on Line 6.Suggested update
+import assert from 'node:assert/strict'; import { test, expect } from 'vitest'; import * as egg from '../src/index.ts'; test('should expose properties', () => { expect(egg).toMatchSnapshot(); - expect(egg.Context).toBeDefined(); - expect(egg.EggAppConfig).toBe(Object); - expect(egg.Logger).toBeDefined(); + assert.ok(egg.Context); + assert.strictEqual(egg.EggAppConfig, Object); + assert.ok(egg.Logger); });As per coding guidelines,
packages/**/test/**/*.test.ts:Use Node.js built-in 'assert' module for assertions in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/index.test.ts` around lines 8 - 9, Replace the two new Jest expect checks with Node's assert-based assertions: import the built-in 'assert' and use assert.strictEqual(egg.EggAppConfig, Object) to mirror expect(...).toBe(Object) and use assert.ok(egg.Logger) (or assert.notStrictEqual(egg.Logger, undefined)) to mirror expect(...).toBeDefined(); keep the existing expect snapshot assertion on line 6 unchanged. Reference the symbols egg.EggAppConfig and egg.Logger and update the test file's imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/egg/src/index.ts`:
- Around line 54-55: The runtime alias for Config is missing which causes
runtime import failures; add a runtime export for EggAppConfig named Config
(i.e., export { EggAppConfig as Config } from './lib/types.ts') placed before
the existing type-only export so that importing { Config } works at runtime just
like the existing export { EggLogger as Logger } ensures Logger is available at
runtime; update the exports in packages/egg/src/index.ts to include this runtime
export alongside the current type export for Config.
---
Outside diff comments:
In `@tools/egg-bundler/src/lib/ExternalsResolver.ts`:
- Around line 53-60: The loop over deps skips calling
`#addMissingOptionalPeerExternals` for packages already set in result (e.g., ones
added by the force set), so optional peers of force-listed packages never get
propagated; to fix, move the await this.#addMissingOptionalPeerExternals(name,
result) call before the if (result[name]) continue check (or right after the
inline/force skip check) so optional peer propagation runs even for force-listed
packages—#addMissingOptionalPeerExternals already guards against duplicate
peers, so this is safe; locate the loop using deps, this.#inline, this.#force,
result and `#shouldExternalize` to apply the change.
---
Nitpick comments:
In `@packages/egg/test/index.test.ts`:
- Around line 8-9: Replace the two new Jest expect checks with Node's
assert-based assertions: import the built-in 'assert' and use
assert.strictEqual(egg.EggAppConfig, Object) to mirror expect(...).toBe(Object)
and use assert.ok(egg.Logger) (or assert.notStrictEqual(egg.Logger, undefined))
to mirror expect(...).toBeDefined(); keep the existing expect snapshot assertion
on line 6 unchanged. Reference the symbols egg.EggAppConfig and egg.Logger and
update the test file's imports accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 489eafa0-39d0-4003-a516-f7e01f577759
⛔ Files ignored due to path filters (3)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snaptools/egg-bundler/test/fixtures/externals/basic-app/node_modules/optional-peer-host/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (9)
packages/egg/src/index.tspackages/egg/src/lib/types.tspackages/egg/test/index.test.tstools/egg-bundler/package.jsontools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/ExternalsResolver.tstools/egg-bundler/test/EntryGenerator.test.tstools/egg-bundler/test/ExternalsResolver.test.tstools/egg-bundler/test/fixtures/externals/basic-app/package.json
💤 Files with no reviewable changes (1)
- tools/egg-bundler/package.json
Contributor
Author
|
Addressed the remaining non-inline CodeRabbit feedback in 5167978: optional peer propagation now also runs for force-listed packages, and the new egg export assertions use node:assert. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/egg/src/index.ts`:
- Line 20: The exported runtime alias Config currently lacks an explicit type,
causing TS9010 under --isolatedDeclarations; update the declaration "export
const Config = EggAppConfig;" to include an explicit type annotation (use
"export const Config: typeof EggAppConfig = EggAppConfig;") so the emitted
declaration can reference the symbol correctly while leaving the separate type
export (the existing type export for EggAppConfig) unchanged.
In `@tools/egg-bundler/docs/output-structure.md`:
- Around line 33-37: The sentence overstated behavior—change the doc text around
ManifestStore.setBundleStore(...) and globalThis.__EGG_BUNDLE_MODULE_LOADER__ so
it no longer claims absolute "no fs.readdir scanning at runtime"; instead state
that installing the bundle loader before startEgg({ baseDir, mode: 'single' })
makes framework module resolution use the inlined bundle map (avoiding
fs.readdir for bundled framework files), but note that application code or
plugins may still perform fs operations (e.g., loading config, views, assets),
so runtime fs access is reduced but not completely eliminated.
In `@tools/egg-bundler/src/lib/ExternalsResolver.ts`:
- Around line 76-81: The peer-resolution loop uses `#findPackageDir`(peerName)
which always starts lookup from this.#baseDir and misses peers resolved from the
dependent package's node_modules; update calls in the loop (where peerName is
iterated) and the other call at the later check to pass the dependent package
directory (pkgDir) as the lookup anchor (e.g., this.#findPackageDir(peerName,
pkgDir)), and modify `#findPackageDir/`#findPackageDirUncached to accept an
optional startDir parameter and use that startDir instead of always using
this.#baseDir so peer lookups correctly resolve from the dependent package's
directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc54f013-953c-4f2b-b53d-258e789009bf
⛔ Files ignored due to path filters (1)
packages/egg/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/egg/src/index.tspackages/egg/test/index.test.tstools/egg-bundler/docs/output-structure.mdtools/egg-bundler/src/lib/ExternalsResolver.tstools/egg-bundler/test/ExternalsResolver.test.tstools/egg-bundler/test/integration.test.ts
✅ Files skipped from review due to trivial changes (1)
- tools/egg-bundler/test/integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/egg/test/index.test.ts
- tools/egg-bundler/test/ExternalsResolver.test.ts
killagu
changed the title
This was referenced May 2, 2026
This was referenced May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the current cnpmcore production bundle blockers seen on next after #5910:
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
Tests